Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to retrieve involved raw types of JavaType and JavaMember #1116

Conversation

leonardhusmann
Copy link
Contributor

@leonardhusmann leonardhusmann commented May 30, 2023

This PR adds two new methods: JavaType.getAllInvolvedRawTypes() and JavaMember.getAllInvolvedRawTypes().
JavaType.getAllInvolvedRawTypes() returns all involved types in this JavaType (e.g. return type of a method)
i.e. for a method public Map<? extends String, ? super Map<Serializable, Object>> method(List<String> input) {...}
the methods should return the following:

  • method.getReturnType().getAllInvolvedRawTypes() would return Set.of(Map.class, String.class, Serializable.class, Object.class)
  • method.getAllInvolvedRawTypes() would return Set.of(Map.class, String.class, Serializable.class, Object.class, List.class)

WDYT? Feedback is highly appreciated :)

Resolves: #723

... which basically disables any automatic wrap for normal use cases.
We leave it open to implementer and reviewer to determine the best way to break lines.
So far this has worked out fine.

Signed-off-by: Peter Gafert <[email protected]>
@codecholeric codecholeric force-pushed the feature/get-involved-JavaClasses-of-signature branch from e65479f to 08e1b40 Compare November 5, 2023 15:55
@codecholeric
Copy link
Collaborator

Thanks a lot for your contribution! 😃 The code looks quite clean and works 🙂
To save some time I simply adjusted the code directly and created two commits from it.

Here are the bigger things I changed:

  • I renamed JavaCodeUnit.getAllTypesInvolvedInSignature() to simply JavaCodeUnit.getAllInvolvedRawTypes(), because IMHO there is no other way how the "raw types involved in a method" could be understood anyway.
    Instead I added some more detailed Javadoc to explain what exactly it means
  • I pulled up JavaCodeUnit.getAllInvolvedRawTypes() to JavaMember.getAllInvolvedRawTypes(), because I think it also makes sense for JavaField (there the signature is basically simply the type) and then it's more unified
  • I made JavaClass.getAllInvolvedRawTypes() return the base component type instead of the class itself, because if the JavaClass represents an array I think we are interested in the component type and not the array type.
    This method is mainly about finding dependencies and the array type is usually not the dependency we're interested in, but the component type. I might be wrong there, maybe it's array type and component type, but I'd wait for
    a concrete issue before adjusting it 😁

But I'm gonna try to give you some feedback on the stuff I noticed so you can hopefully also take some stuff away 🙂

First thing I noticed was the commit structure. For ArchUnit we try to create self-contained logical units.
At least to be ready to merge, the history shouldn't look like "this is what I did in which order", but "these are logical, cohesive and self-contained steps to reach the goal".
These steps should be as small as possible, but still contain a logical unit. For example, a commit "add tests" i.g. doesn't make any sense IMHO.
Because it means the commits before were incomplete. For me the tests are an inseparable requirement for the implementation.
You should be able to check out any commit and the code base in this state should be complete, reasonable and fully tested.

Another thing I noticed was that Javadoc was missing. We don't create Javadoc in a cargo cult way, but every non-trivial method should get it.
And "involved raw types" isn't self-explanatory enough from my point of view to be understood i.g. without any Javadoc.
BTW: Public methods also need an explicit annotation @PublicAPI or the tests will fail 😉

Regarding the tests, there are a lot of custom assertions that make your life simpler. We never use JUnit assertEquals, but only AssertJ, since it's nicer to read and offers autocomplete. But for all domain objects where we have recurring assertions we have custom matchers, e.g. assertThatTypes(..) which then directly allows to assert class objects.
I also thought that in some places the test cases could have been a little more complex, so I adjusted this.
You could also try test driven development for these nice self-contained cases, because for some code I saw that test cases were missing.
E.g. you implemented stuff in JavaGenericArrayType, but I didn't see any test with a generic array. If you would do this test driven, then such a thing couldn't happen 🤷‍♂️
Also, in tests you don't need to import every single type involved, because ArchUnit has an automatic resolution process that will import transitive stuff as needed. So it's always worth a shot if importClass(Single.class) is maybe already good enough for the test case.

Last but not least I saw quite some unrelated code reformatting. IMHO you shouldn't touch unrelated code like this, definitely not in the same commit as "real" changes, cause it just makes the review harder (and reasoning about the change later on as well).
If you think some formatting is suboptimal we can discuss it and potentially adjust it, but it should at least be a completely separate commit then with a message explicitly describing that this is a pure formatting change.
Nevertheless, I also saw that a fresh import of the IntelliJ code style meanwhile sets hard wrap by default to 150 chars 🙈
This was different in earlier times, so I adjusted the code formatter for IntelliJ in a pre-commit to fix the hard wrap to something so big that we effectively just decide about the line breaks.
Not sure if this is i.g. the best thing, or if we should have a more restrictive hard wrap, but so far I also don't remember this to caues any problems 🤷‍♂️

BTW: We also don't use any branch prefix convention 😁 E.g. in the PR title...

@codecholeric codecholeric changed the title Feature/get involved java classes of signature Allow to retrieve involved raw types of JavaType and JavaMember Nov 5, 2023
Adds a convenience method to quickly determine all `JavaClass`es any `JavaType` depends on. For a complex type, like the parameterized type `Map<? extends Serializable, List<? super Set<String>>>`, this can otherwise be quite tedious and possibly make it necessary to traverse the whole type parameter hierarchy.

Issue: TNG#723
Signed-off-by: Leonard Husmann <[email protected]>
Signed-off-by: Peter Gafert <[email protected]>
Convenience method to find all `JavaClass`es involved in any member's signature. For a field these are only the raw types involved in the field type, for methods and constructors it's the union of all raw types involved in parameter types, return types and type parameters.

Issue: TNG#723
Signed-off-by: Leonard Husmann <[email protected]>
Signed-off-by: Peter Gafert <[email protected]>
@codecholeric codecholeric force-pushed the feature/get-involved-JavaClasses-of-signature branch from 08e1b40 to ea320d8 Compare November 5, 2023 17:48
@leonardhusmann
Copy link
Contributor Author

Wow! thanks a lot for this extensive feedback 😄🙏🙏

@codecholeric codecholeric merged commit d6030be into TNG:main Nov 5, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support an easy way to obtain all involved JavaClasses of a generic signature
2 participants